libcontainer: Allow passing mount propagation flags #264

rhvgoyal · 2015-09-11T16:48:05Z

Fixes issue #263

Right now if one passes a mount propagation flag in spec file, it
does not take effect. For example, try following in spec json file.

{
"type": "bind",
"source": "/root/mnt-source",
"destination": "/root/mnt-dest",
"options": "rbind,shared"
}

One would expect that /root/mnt-dest will be shared inside the container
but that's not the case.

findmnt -o TARGET,PROPAGATION

`-/root/mnt-dest private

Reason being that propagation flags can't be passed in along with other
regular flags. They need to be passed in a separate call to mount syscall.
That too, one propagation flag at a time. (from mount man page).

Hence, store propagation flags separately in a slice and apply these
in that order after the mount call wherever appropriate. This allows
user to control the propagation property of mount point inside
the container.

Storing them separately also solves another problem where recursive flag
(syscall.MS_REC) can get mixed up. For example, options "rbind,private"
and "bind,rprivate" will be same and there will be no way to differentiate
between these if all the flags are stored in a single integer.

This patch would allow one to pass propagation flags "[r]shared,[r]slave,
[r]private,[r]unbindable" in spec file as per mount property.

Signed-off-by: Vivek Goyal vgoyal@redhat.com

mrunalp · 2015-09-11T16:50:39Z

@LK4D4 @crosbymichael @dqminh PTAL.
Going for a step by step approach towards volume propagation to keep it simple.

LK4D4 · 2015-09-11T19:53:59Z

libcontainer/rootfs_linux.go

@@ -107,12 +121,12 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error {
 		if err := os.MkdirAll(dest, 0755); err != nil {
 			return err
 		}
-		return syscall.Mount(m.Source, dest, m.Device, uintptr(m.Flags), "")
+		return mountPropagate(m.Source, dest, m.Device, m.Flags, "", m.PropagationFlags)


at first I don't know why we ignored m.Data whole time. Second, maybe we can make it just method of configs.Mount? like m.Mount(rootfs), dunno, what to do with mountLabel though.

we seem to be ignoring m.Data only for /proc/, /sys and mqueue. m.Data is used to pass file system specific options. In mount man page I don't see any filesystem specific options mentioned for mqueue or for sysfs. For "procfs" looks like it recognizes uid= and gid= options but does nothing. Following is from "mount" man page.

"Mount options for proc
uid=value and gid=value
These options are recognized, but have no effect as far as I can
see.
"

So ignoring m.Data for proc, sysfs, and mqueue might not be hurting in practice.

mountLabel is passed as "context=" mount option. As per the mount man page, it is file system independent option. That means it should be passed in as part of m.Flags and not part of m.Data. This is strange. What am I missing.

I dunno as well :)

Following is the syntax of mount syscall.

int mount(const char *source, const char *target, const char *filesystemtype, unsigned long mountflags, const void *data);

mountflags is just an unsigned long. That means it can only take certain predefined flags. Rest of the string style options have to go through "data". So context= has to go through data string. So it is fine. And docker seems to be doing same thing.

As we seem to be modifying both m.Dest and m.Data (if need be), I would rather leave it as it is and not try to define a method on configs.Mount().

Anyway, that can be done later as part of cleanup and should not be a blocker for this patch.

So, I'm not sure now that mountLabel belongs to libcontainer at all.
But I still want to have mount.Mount method, maybe it can accept rootfs and mountlabel for now. Because it's sorta tiring to write all this struct fields each time.

We modify m.Destination as well. So that means we will accept rootfs, dest and mountlabel. I find it litle strange. In the method we will allow overrideing m.Destination and m.Data but not other field. But if you are so particular about this method, I can implement it. Let me know.

rhvgoyal · 2015-09-14T14:20:21Z

@LK4D4 I have introduced a method MountPropagate() now. PTAL.

mrunalp · 2015-09-14T20:55:40Z

Need rebase.

Right now if one passes a mount propagation flag in spec file, it does not take effect. For example, try following in spec json file. { "type": "bind", "source": "/root/mnt-source", "destination": "/root/mnt-dest", "options": "rbind,shared" } One would expect that /root/mnt-dest will be shared inside the container but that's not the case. #findmnt -o TARGET,PROPAGATION `-/root/mnt-dest private Reason being that propagation flags can't be passed in along with other regular flags. They need to be passed in a separate call to mount syscall. That too, one propagation flag at a time. (from mount man page). Hence, store propagation flags separately in a slice and apply these in that order after the mount call wherever appropriate. This allows user to control the propagation property of mount point inside the container. Storing them separately also solves another problem where recursive flag (syscall.MS_REC) can get mixed up. For example, options "rbind,private" and "bind,rprivate" will be same and there will be no way to differentiate between these if all the flags are stored in a single integer. This patch would allow one to pass propagation flags "[r]shared,[r]slave, [r]private,[r]unbindable" in spec file as per mount property. Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

mrunalp · 2015-09-17T17:19:59Z

@LK4D4 @crosbymichael @dqminh PTAL

crosbymichael · 2015-09-17T21:58:11Z

LGTM

LK4D4 · 2015-09-17T22:02:14Z

LGTM

libcontainer: Allow passing mount propagation flags

mrunalp · 2015-09-17T22:03:26Z

Thanks @LK4D4 @crosbymichael :)
@rhvgoyal Onto the next step! :)

rhvgoyal · 2015-09-18T01:11:42Z

Thanks folks for merging this.

@mrunalp yes, onto the next step now. :-)

ibuildthecloud · 2015-09-23T03:46:22Z

Oops, mixed up this PR with #208, well shoot, I was all excited #208 was merged but I guess not...

rhvgoyal · 2015-09-23T11:47:38Z

@ibuildthecloud #208 is not merged yet. A small infrastructure piece has been merged. I am reworking my patches and create another PR.

GordonTheTurtle added the status/0-triage label Sep 11, 2015

LK4D4 reviewed Sep 11, 2015
View reviewed changes

rhvgoyal force-pushed the spec-allow-propgation-flags branch 2 times, most recently from 2065b83 to 0a8bdd4 Compare September 14, 2015 14:13

rhvgoyal force-pushed the spec-allow-propgation-flags branch from 0a8bdd4 to b7d7718 Compare September 14, 2015 22:06

rhvgoyal force-pushed the spec-allow-propgation-flags branch from b7d7718 to d1f4a5b Compare September 16, 2015 19:53

LK4D4 added a commit that referenced this pull request Sep 17, 2015

Merge pull request #264 from rhvgoyal/spec-allow-propgation-flags

cae6b31

libcontainer: Allow passing mount propagation flags

LK4D4 merged commit cae6b31 into opencontainers:master Sep 17, 2015

mrunalp mentioned this pull request Sep 18, 2015

Mount propagation flags don't work #263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libcontainer: Allow passing mount propagation flags #264

libcontainer: Allow passing mount propagation flags #264

rhvgoyal commented Sep 11, 2015

mrunalp commented Sep 11, 2015

LK4D4 Sep 11, 2015

rhvgoyal Sep 11, 2015

rhvgoyal Sep 11, 2015

LK4D4 Sep 11, 2015

rhvgoyal Sep 11, 2015

rhvgoyal Sep 11, 2015

LK4D4 Sep 11, 2015

rhvgoyal Sep 11, 2015

rhvgoyal commented Sep 14, 2015

mrunalp commented Sep 14, 2015

mrunalp commented Sep 17, 2015

crosbymichael commented Sep 17, 2015

LK4D4 commented Sep 17, 2015

mrunalp commented Sep 17, 2015

rhvgoyal commented Sep 18, 2015

ibuildthecloud commented Sep 23, 2015

rhvgoyal commented Sep 23, 2015

libcontainer: Allow passing mount propagation flags #264

libcontainer: Allow passing mount propagation flags #264

Conversation

rhvgoyal commented Sep 11, 2015

findmnt -o TARGET,PROPAGATION

mrunalp commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhvgoyal commented Sep 14, 2015

mrunalp commented Sep 14, 2015

mrunalp commented Sep 17, 2015

crosbymichael commented Sep 17, 2015

LK4D4 commented Sep 17, 2015

mrunalp commented Sep 17, 2015

rhvgoyal commented Sep 18, 2015

ibuildthecloud commented Sep 23, 2015

rhvgoyal commented Sep 23, 2015